refactor(cli): split cli_app into focused submodules#21
Conversation
Extract Typer Annotated aliases (cli_types), Rich dashboard and list output (cli_dashboard), RuntimeReporter thread (cli_reporter), and LiveKit argv/env handoff plus pool reporting (cli_livekit). cli_app keeps command definitions and main(); re-exports public symbols for existing imports. Tests patch AgentPool on cli_app for list and cli_livekit for worker commands. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
…tions Introduce cli_params with agent_provider_kwargs and a frozen dataclass used by cli_livekit delegates. Register start/dev/console via a single handler factory to avoid repeating identical Typer option lists. Add focused unit tests. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
==========================================
+ Coverage 90.10% 90.22% +0.11%
==========================================
Files 7 13 +6
Lines 1274 1361 +87
==========================================
+ Hits 1148 1228 +80
- Misses 126 133 +7
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Refactors the OpenRTC Typer-based CLI by extracting previously monolithic cli_app functionality into focused submodules, while keeping cli_app as the command registration + programmatic main() entrypoint and preserving compatibility via re-exports.
Changes:
- Split shared Typer
Annotatedoption aliases intocli_types, and Rich rendering/output helpers intocli_dashboard. - Moved LiveKit argv/env handoff and pool lifecycle helpers into
cli_livekit, and the runtime metrics background thread intocli_reporter. - Updated CLI tests to monkeypatch
AgentPoolin the new module locations for worker commands.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_cli.py | Updates monkeypatch targets to reflect the new module split (notably cli_livekit.AgentPool). |
| src/openrtc/cli_types.py | New module centralizing Typer Annotated option aliases and CLI help panel constants. |
| src/openrtc/cli_dashboard.py | New module for Rich dashboard rendering, list output, and resource summary/JSON payload building. |
| src/openrtc/cli_reporter.py | New module for the background runtime reporter (dashboard/JSON/JSONL). |
| src/openrtc/cli_livekit.py | New module for LiveKit delegation: argv stripping, env overrides, discovery, and pool/reporting lifecycle. |
| src/openrtc/cli_app.py | Slimmed down to command definitions + main(), with re-exports for compatibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from openrtc.cli_dashboard import ( | ||
| build_list_json_payload, | ||
| build_runtime_dashboard, | ||
| print_list_plain, | ||
| print_list_rich_table, | ||
| print_resource_summary_rich, | ||
| ) |
There was a problem hiding this comment.
Several imports in this module appear to exist only for re-exporting (e.g. build_runtime_dashboard, RuntimeReporter, _run_pool_with_reporting, _strip_openrtc_only_flags_for_livekit). Ruff/pyflakes will still flag these as F401 (imported but unused) because listing names in all doesn’t count as usage. Consider adding per-import # noqa: F401 (or an explicit re-export assignment pattern) so CI lint passes while keeping the public API stable.
| _run_pool_with_reporting, | ||
| _strip_openrtc_only_flags_for_livekit, |
There was a problem hiding this comment.
These imports include helpers that are only re-exported (e.g. _run_pool_with_reporting, _strip_openrtc_only_flags_for_livekit). As written they are not referenced in code, so Ruff will report F401 unused imports. Add # noqa: F401 on the specific imports or switch to an explicit re-export (e.g. importing the module and assigning names) to avoid lint failures.
| _run_pool_with_reporting, | |
| _strip_openrtc_only_flags_for_livekit, | |
| _run_pool_with_reporting, # noqa: F401 re-exported via __all__ | |
| _strip_openrtc_only_flags_for_livekit, # noqa: F401 re-exported via __all__ |
| from openrtc.cli_reporter import RuntimeReporter | ||
| from openrtc.cli_types import ( |
There was a problem hiding this comment.
RuntimeReporter is imported here but not referenced (it’s only listed in all). Ruff will flag this as an unused import (F401). Add a targeted # noqa: F401 (or use an explicit alias assignment re-export) to keep the re-export without breaking lint.
Introduce openrtc.provider_types.ProviderValue (str | Any) documenting provider ID strings vs LiveKit plugin instances. Use it on AgentConfig, AgentDiscoveryConfig, agent_config(), AgentPool defaults/add(), and CLI worker option bundles. Export ProviderValue from the package root. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Use isinstance(openai.NotGiven) when importable; fall back to class name plus openai module prefix. Removes fragile repr(value) == "NOT_GIVEN" check. Add regression tests for both sentinels and unrelated NotGiven classes. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Replace chained module/qualname checks with _PROVIDER_REF_KEYS frozenset and a single _try_build_provider_ref path. Document extending the set for new LiveKit plugins that use the same _opts flattening contract. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
- Add mypy step to lint.yml after installing editable package + cli/tui extras (matches livekit imports; pyproject already sets ignore_missing_imports). - Resolve prior mypy issues: OpenAI NotGiven import typing, AgentSession annotation, discovery metadata cast, RuntimeMetricsStore pickle __setstate__ narrowing, JsonlMetricsSink.write_event Mapping payload, _format_percent guards, async Textual action_quit + test await. - Document in AGENTS.md and CONTRIBUTING.md. Avoid 'mypy ... || true' so CI actually blocks regressions. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Module docstring ties stubs to pyproject livekit-agents pin, drift risk, and when to extend stubs. CONTRIBUTING and AGENTS clarify real SDK is default with uv sync; shim is ImportError fallback. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
Empty marker lives at src/openrtc/py.typed; hatch includes it in wheel and sdist. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
tui_app is covered by tests/test_tui_app.py; project coverage remains above the 80% gate with it included. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
onnxruntime 1.24+ has no cp310 wheels; LiveKit plugins still depend on it. Add a PEP 508 conditional dependency (1.23.x on <3.11) so uv add/sync and pip install work on 3.10. Regenerate uv.lock; document in CONTRIBUTING. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
- README: uv install, py.typed, ProviderValue, expanded src layout, onnxruntime 3.10 note, CI Ruff/mypy - getting-started: uv, PEP 561, Python 3.10/onnxruntime pointer - cli: document split cli_* modules - api/pool: ProviderValue types replace Any in signatures - architecture: mention ProviderValue for pipeline slots VitePress docs:build succeeds. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
onnxruntime and the LiveKit Silero/turn-detector stack no longer support 3.10 wheels in current releases; the conditional onnxruntime pin was fragile. - requires-python >=3.11,<3.14; remove onnxruntime extra constraint - CI test matrix and PyPI publish Python updated; mypy python_version 3.11 - Docs, README, CONTRIBUTING, AGENTS, skill: version notes BREAKING CHANGE: Python 3.10 environments must upgrade to 3.11 or newer. Co-authored-by: Mahimai Raja J <mahimairaja3@gmail.com>
|
@cursor review |
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/openrtc/provider_types.py
Outdated
| # ``Any`` covers third-party plugin classes without enumerating them here; use | ||
| # strings when you want the type checker to stay precise. | ||
| ProviderValue: TypeAlias = str | Any |
There was a problem hiding this comment.
ProviderValue is defined as str | Any, but Any absorbs unions (type checkers treat str | Any as Any). That makes ProviderValue effectively untyped and undermines the intent of keeping string-based configuration precise. Consider using str | object (or a Protocol/base class for provider instances) so the alias remains meaningful while still accepting arbitrary plugin instances.
| # ``Any`` covers third-party plugin classes without enumerating them here; use | |
| # strings when you want the type checker to stay precise. | |
| ProviderValue: TypeAlias = str | Any | |
| # Arbitrary plugin instances are typed as ``object`` to allow third-party classes | |
| # without enumerating them here; use strings when you want the type checker to | |
| # stay precise. | |
| ProviderValue: TypeAlias = str | object |
tests/conftest.py
Outdated
| from __future__ import annotations | ||
|
|
||
| """Pytest configuration and shared fixtures. | ||
|
|
There was a problem hiding this comment.
The module docstring is placed after from __future__ import annotations, so it won’t be recognized as the module docstring (it becomes a no-op string expression). Move the docstring to the top of the file (before the future import) to keep tooling/documentation behavior consistent.
| def _is_not_given(value: Any) -> bool: | ||
| return repr(value) == "NOT_GIVEN" | ||
| """True if ``value`` is OpenAI's ``NotGiven`` (unset optional on plugin ``_opts``).""" | ||
| if _OPENAI_NOT_GIVEN_TYPE is not None and isinstance(value, _OPENAI_NOT_GIVEN_TYPE): |
There was a problem hiding this comment.
This if condition line is longer than typical Ruff formatting limits and will likely fail ruff format --check . in CI. Reformat by splitting the condition across multiple lines (the formatter would typically do this automatically).
| if _OPENAI_NOT_GIVEN_TYPE is not None and isinstance(value, _OPENAI_NOT_GIVEN_TYPE): | |
| if ( | |
| _OPENAI_NOT_GIVEN_TYPE is not None | |
| and isinstance(value, _OPENAI_NOT_GIVEN_TYPE) | |
| ): |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@mahimairaja I've opened a new pull request, #22, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ng, pool.py format Co-authored-by: mahimairaja <81288263+mahimairaja@users.noreply.github.com> Agent-Logs-Url: https://github.com/mahimairaja/openrtc-python/sessions/8aca67d0-da08-4745-899f-296a8db94b8b
fix: correct ProviderValue typing, conftest docstring placement, and pool.py line formatting
Extract Typer Annotated aliases (cli_types), Rich dashboard and list output (cli_dashboard), RuntimeReporter thread (cli_reporter), and LiveKit argv/env handoff plus pool reporting (cli_livekit). cli_app keeps command definitions and main(); re-exports public symbols for existing imports.
Tests patch AgentPool on cli_app for list and cli_livekit for worker commands.